Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update contract #2314

Merged
merged 11 commits into from
Nov 12, 2024
Merged

Update contract #2314

merged 11 commits into from
Nov 12, 2024

Conversation

nick-bisonai
Copy link
Collaborator

@nick-bisonai nick-bisonai commented Nov 8, 2024

Description

There have been a client request with submission proxy contract
This update tries to fix following scenario

  1. user a and user b tries to submit in a same block.timestamp 10
  2. user a received dal timestamp with 8 while user b received dal timestamp with 9
  3. when user a and user b submits at the same time, if the execution order becomes b->a, user a fails to submit the price.

if the submission fails, client can't tell whether it is caused by data freshness failure, or because if there was a newer submission ahead.

it is hard for client to make decision with the latest round data even if the submission error is ignored

there are 2 approaches that could be taken

  1. make lastSubmissionTimes public so that client can utilize lastSubmissionTimes
  2. add function that only updates if the data is newer, if not, just ignore

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.

Deployment

  • Should publish npm package
  • Should publish Docker image

@nick-bisonai nick-bisonai self-assigned this Nov 8, 2024
@nick-bisonai nick-bisonai requested a review from a team as a code owner November 8, 2024 12:52
Copy link
Contributor

coderabbitai bot commented Nov 8, 2024

Warning

Rate limit exceeded

@nick-bisonai has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 36 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 7109bcf and 2e966a5.

📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

The SubmissionProxy contract has been updated to improve its functionality and error handling. The visibility of the thresholds and lastSubmissionTimes mappings has been changed to public, allowing external access. Two new functions, updatePrices and updatePrice, have been introduced to handle multiple and individual price submissions, respectively. Error handling has been refined with the renaming of AnswerTooOld to AnswerOutdated and the addition of AnswerSuperseded. Control flow adjustments in the submitStrict function ensure better management of submissions.

Changes

File Change Summary
contracts/v0.2/src/SubmissionProxy.sol - Updated visibility of thresholds mapping to public.
- Updated visibility of lastSubmissionTimes mapping to public.
- Added updatePrices function for batch price submissions.
- Added updatePrice function for individual price updates.
- Renamed submitStrict to submitStrictBatch and updated documentation.
- Renamed submitSingle to submitStrictSingle and updated documentation.
- Renamed error AnswerTooOld to AnswerOutdated and added AnswerSuperseded.
contracts/v0.2/test/SubmitStrict.t.sol - Updated method call from submitStrict to submitStrictBatch for batch submission handling.

Possibly related PRs

  • (Contract) Add check based on last submission time #1506: This PR adds a new mapping for lastSubmissionTimes and updates submission logic to check and update the last submission time, which directly relates to the changes made in the main PR regarding the handling of submission timestamps.
  • (Contract) SubmissionProxy update #1890: This PR introduces new error types and functions related to submission handling, including checks for submission timestamps, which aligns closely with the modifications in the main PR that enhance error handling and submission validation.
  • Review of superseded feature #2315: This PR reviews changes in the SubmissionProxy that include updates to mapping visibility and error handling, which are also key aspects of the main PR's modifications.

Suggested reviewers

  • martinkersner
  • Intizar-T

🐇 In the meadow, submissions flow,
With new functions, watch them grow!
Public mappings, clear as day,
Outdated timestamps kept at bay.
A rabbit's hop, a leap so bright,
Efficient paths, all done just right! 🌼


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
contracts/v0.2/src/SubmissionProxy.sol (1)

467-499: Consider adding a return value for better client feedback.

The implementation correctly handles the submission order issue by:

  1. First checking if the submission is newer
  2. Then validating freshness and other requirements

However, the silent return for outdated submissions might be confusing for clients. Consider returning a boolean to indicate whether the submission was processed:

-function submitSingleIfNew(bytes32 _feedHash, int256 _answer, uint256 _timestamp, bytes calldata _proof) public {
+function submitSingleIfNew(bytes32 _feedHash, int256 _answer, uint256 _timestamp, bytes calldata _proof) public returns (bool) {
     if (lastSubmissionTimes[_feedHash] >= _timestamp) {
-        return;
+        return false;
     }
     // ... rest of the function ...
     if (validateProof(_feedHash, message_, proofs_)) {
         feeds[_feedHash].submit(_answer);
         lastSubmissionTimes[_feedHash] = _timestamp;
+        return true;
     } else {
         revert InvalidProof();
     }
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 1625938 and f026313.

📒 Files selected for processing (1)
  • contracts/v0.2/src/SubmissionProxy.sol (2 hunks)
🔇 Additional comments (2)
contracts/v0.2/src/SubmissionProxy.sol (2)

40-41: LGTM: Enhanced visibility improves client accessibility.

The change to make these mappings public is a good improvement as it:

  • Allows clients to query last submission times, helping them make informed decisions about submissions
  • Provides transparency about threshold values

448-465: LGTM: Well-structured batch submission function.

The implementation:

  • Maintains consistent input validation with other submit functions
  • Efficiently delegates to submitSingleIfNew for individual submissions
  • Addresses the PR objective by handling only newer submissions

Copy link
Member

@martinkersner martinkersner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I think we can go only with making the mappings public, rather than creating a new submission methods 🙏

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 345ef36 and 17d5b19.

📒 Files selected for processing (1)
  • contracts/v0.2/src/SubmissionProxy.sol (3 hunks)
🔇 Additional comments (3)
contracts/v0.2/src/SubmissionProxy.sol (3)

40-41: LGTM! Visibility changes improve transparency

Making these mappings public allows clients to query submission times and thresholds directly, helping them understand submission failures better.


64-65: LGTM! Improved error handling with specific error types

The new error types AnswerOutdated and AnswerSuperseded provide clear distinction between different failure scenarios, helping clients better understand why their submissions failed.


424-425: LGTM! Clear handling of superseded submissions

The check for superseded submissions is correct and uses consistent timestamp units. The error provides clear feedback when a newer submission has already been processed.

contracts/v0.2/src/SubmissionProxy.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
contracts/v0.2/src/SubmissionProxy.sol (1)

420-451: Consider extracting common validation logic.

Both submitSingle and updatePrice share significant validation logic. Consider extracting these common validations into a private function to improve maintainability and reduce code duplication.

Example refactor:

+    function validateSubmission(bytes32 _feedHash, int256 _answer, uint256 _timestamp, bytes calldata _proof) 
+        private 
+        returns (bytes[] memory proofs_) 
+    {
+        if (_timestamp <= ((block.timestamp - dataFreshness) * 1000)) {
+            revert AnswerOutdated();
+        }
+
+        (proofs_, bool success_) = splitProofs(_proof);
+        if (!success_) {
+            revert InvalidProofFormat();
+        }
+
+        if (address(feeds[_feedHash]) == address(0)) {
+            revert FeedHashNotFound();
+        }
+
+        if (keccak256(abi.encodePacked(feeds[_feedHash].name())) != _feedHash) {
+            revert InvalidFeedHash();
+        }
+        
+        return proofs_;
+    }

Also applies to: 469-492

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 17d5b19 and d68433e.

📒 Files selected for processing (1)
  • contracts/v0.2/src/SubmissionProxy.sol (3 hunks)
🔇 Additional comments (5)
contracts/v0.2/src/SubmissionProxy.sol (5)

40-41: LGTM: Public mappings enhance transparency.

The change to make thresholds and lastSubmissionTimes mappings public aligns with the PR objective of allowing clients to access this information for better decision-making.


64-65: LGTM: Improved error handling with descriptive errors.

The renaming from AnswerTooOld to AnswerOutdated and addition of AnswerSuperseded provides clearer distinction between different failure scenarios:

  • AnswerOutdated: Data is older than the freshness threshold
  • AnswerSuperseded: A newer submission was already processed

453-466: LGTM: New updatePrices function handles superseded data gracefully.

The updatePrices function provides a more lenient approach by:

  1. Continuing execution when answers are superseded (via updatePrice)
  2. Still maintaining data integrity by validating proofs
  3. Properly handling other error cases

469-475: LGTM: updatePrice implements the desired behavior.

The updatePrice function successfully addresses the PR objective by:

  1. Still rejecting outdated data (older than freshness threshold)
  2. Silently skipping superseded data instead of reverting
  3. This allows clients to better understand why their submissions failed

420-426: ⚠️ Potential issue

Timestamp comparison needs parentheses for clarity.

The timestamp comparison could be clearer with proper parentheses grouping.

Apply this fix:

-if (_timestamp <= (block.timestamp - dataFreshness) * 1000 ) {
+if (_timestamp <= ((block.timestamp - dataFreshness) * 1000)) {

contracts/v0.2/src/SubmissionProxy.sol Outdated Show resolved Hide resolved
* chore: superseeded review

* fix: fix test

---------

Co-authored-by: nick <[email protected]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
contracts/v0.2/test/SubmitStrict.t.sol (1)

Line range hint 30-35: Consider parameterizing contract addresses.

The test uses hardcoded addresses for both SubmissionProxy and FeedRouter contracts. This makes the test brittle and environment-dependent.

Consider:

  1. Moving addresses to a configuration file
  2. Using constructor parameters
  3. Implementing a test fixture that deploys fresh contract instances

Example approach:

contract SubmitStrictTest is Test {
    SubmissionProxy public sp;
    FeedRouter public fr;
    
    function setUp() public {
        // Deploy or load from config
        sp = new SubmissionProxy();
        fr = new FeedRouter();
    }
}

Also applies to: 39-40

contracts/v0.2/src/SubmissionProxy.sol (1)

Line range hint 509-544: Fix potential race condition in updatePrice.

There's a potential race condition between the initial timestamp check and the final submission. The second check at lines 538-540 is correct but could be optimized.

Apply this fix to remove the redundant check and prevent the race condition:

    function updatePrice(bytes32 _feedHash, int256 _answer, uint256 _timestamp, bytes calldata _proof) public {
        if (lastSubmissionTimes[_feedHash] >= _timestamp) {
            // answer is superseeded -> skip submission
            return;
        }

        if (_timestamp <= (block.timestamp - dataFreshness) * 1000) {
            // answer is not fresh -> do not submit!
            revert AnswerOutdated();
        }

        (bytes[] memory proofs_, bool success_) = splitProofs(_proof);
        if (!success_) {
            // splitting proofs failed -> do not submit!
            revert InvalidProofFormat();
        }

        if (address(feeds[_feedHash]) == address(0)) {
            // feedHash not registered -> do not submit!
            revert FeedHashNotFound();
        }

        if (keccak256(abi.encodePacked(feeds[_feedHash].name())) != _feedHash) {
            // feedHash not matching with registered feed -> do not submit!
            revert InvalidFeedHash();
        }

        bytes32 message_ = keccak256(abi.encodePacked(_answer, _timestamp, _feedHash));
        if (validateProof(_feedHash, message_, proofs_)) {
-           if (lastSubmissionTimes[_feedHash] >= _timestamp) {
-               return;
-           }
            feeds[_feedHash].submit(_answer);
            lastSubmissionTimes[_feedHash] = _timestamp;
        } else {
            revert InvalidProof();
        }
    }

The redundant check at lines 538-540 should be removed as it creates a race condition window between the initial check and the final submission. The initial check at the start of the function is sufficient.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 72f7c1f and 254b7e9.

📒 Files selected for processing (2)
  • contracts/v0.2/src/SubmissionProxy.sol (5 hunks)
  • contracts/v0.2/test/SubmitStrict.t.sol (1 hunks)
🔇 Additional comments (8)
contracts/v0.2/test/SubmitStrict.t.sol (2)

Line range hint 30-35: Method name updated to reflect batch functionality.

The change from submitStrict to submitStrictBatch better reflects the batch nature of the operation. However, with the recent changes to the SubmissionProxy contract that add new functions for handling submissions, we should ensure comprehensive test coverage.

Let's verify the test coverage for the new functionality:


Line range hint 46-49: Verify timestamp validation for the new submission scenarios.

Given the PR's focus on timestamp-related submission issues, ensure the timestamp validation in the test adequately covers the edge cases mentioned in the PR objectives (user a/b submitting at timestamps 8/9).

Let's check for related test cases:

contracts/v0.2/src/SubmissionProxy.sol (6)

40-41: LGTM! Public mappings improve transparency.

The change to make thresholds and lastSubmissionTimes mappings public aligns with the PR objective of allowing clients to access submission timing information.


64-65: LGTM! Improved error naming and handling.

The renaming from AnswerTooOld to AnswerOutdated and addition of AnswerSuperseded provides clearer error distinction between outdated timestamps and superseded submissions.


Line range hint 400-426: LGTM! Clear batch submission with strict validation.

The submitStrictBatch function provides a clean interface for submitting multiple prices with proper validation through submitStrictSingle.


436-470: LGTM! Comprehensive validation in submitStrictSingle.

The function properly validates:

  1. Timestamp freshness
  2. Submission order
  3. Proof format
  4. Feed registration
  5. Feed hash matching
  6. Proof validation

481-496: LGTM! Non-strict batch update implementation.

The updatePrices function provides a more flexible alternative that skips superseded submissions instead of reverting the entire batch.


437-437: Verify timestamp comparison units.

The timestamp comparisons in both submitStrictSingle and updatePrice functions need parentheses for clarity, as flagged in past reviews:

Apply this fix to both occurrences:

-if (_timestamp <= (block.timestamp - dataFreshness) * 1000) {
+if (_timestamp <= ((block.timestamp - dataFreshness) * 1000)) {

Also applies to: 515-515

@nick-bisonai
Copy link
Collaborator Author

do we need tests for the new updatePrices functions?

will be good to have, but since the logic is pretty much same as submitStrict but with one less condition, I considered it not necessary

@nick-bisonai nick-bisonai merged commit 0b8ff1f into master Nov 12, 2024
1 check passed
@nick-bisonai nick-bisonai deleted the feat/update-based-on-feedback branch November 12, 2024 06:01
@coderabbitai coderabbitai bot mentioned this pull request Nov 12, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants